fix: handle virtual providers in forbid registration - #293
Conversation
|
It looks like you already got the gist of it, but in case it's helpful, here's the larger context from my end (as produced by claude w/ a reproducer for A version set satisfied by candidates from multiple package names trips a debug assert (breaks virtual provides)Summary
at Root causeThe "force at most one solvable per package name" logic derives the name from for (&candidates, variables) in candidates.iter().zip(version_set_variables.iter()) {
let Some(&first_solvable) = candidates.first() else {
continue;
};
let name_id = self.cache.provider().solvable_name(first_solvable);
debug_assert!(
candidates
.iter()
.all(|&solvable| self.cache.provider().solvable_name(solvable) == name_id),
"all candidates in a version set must have the same package name"
);
if self.state.allow_multiple_names.contains(name_id) {
continue;
}
let pending = self.pending_forbid_clauses.entry(name_id).or_default();
for &variable_id in variables {
// ... all providers bucketed under `name_id` ...
}
}This holds for ecosystems where a requirement names exactly one package (conda), Impact
Suggested fix directionInstead of assuming one name per version set, bucket the candidates by their for (&candidates, variables) in candidates.iter().zip(version_set_variables.iter()) {
for (&solvable, &variable_id) in candidates.iter().zip(variables.iter()) {
let name_id = self.cache.provider().solvable_name(solvable);
if self.state.allow_multiple_names.contains(name_id) {
continue;
}
// ... push `variable_id` into pending_forbid_clauses[name_id] ...
}
}(Exact shape depends on how Reproducer (standalone, no RPM data)Minimal //! Minimal reproducer: a single version set whose candidates come from more
//! than one package name (a virtual capability provided by different packages)
//! trips the `debug_assert!` in `on_requirement_candidates_available`
//! ("all candidates in a version set must have the same package name").
use std::{any::Any, fmt::Display};
use resolvo::{
Candidates, Condition, ConditionId, Dependencies, DependencyProvider, Interner,
KnownDependencies, NameId, Problem, SolvableId, Solver, SolverCache, StringId, VersionSetId,
VersionSetUnionId, utils::Pool,
};
use version_ranges::Ranges;
/// A provider where a capability name can be provided by solvables interned
/// under *different* package names (virtual provides, as in RPM).
#[derive(Default)]
struct ProvidesProvider {
pool: Pool<Ranges<u32>>,
/// capability name -> list of providing solvables.
provides: ahash::HashMap<NameId, Vec<SolvableId>>,
}
impl ProvidesProvider {
/// Register that package `pkg` (version `ver`) provides `capability`.
fn add_provider(&mut self, pkg: &str, ver: u32, capability: &str) {
let pkg_name = self.pool.intern_package_name(pkg);
let solvable = self.pool.intern_solvable(pkg_name, ver);
let cap_name = self.pool.intern_package_name(capability);
self.provides.entry(cap_name).or_default().push(solvable);
// A package also provides itself.
self.provides.entry(pkg_name).or_default().push(solvable);
}
fn version_set(&self, capability: &str) -> VersionSetId {
let name = self.pool.intern_package_name(capability);
self.pool.intern_version_set(name, Ranges::full())
}
}
impl Interner for ProvidesProvider {
type NameId = NameId;
type SolvableId = SolvableId;
fn display_solvable(&self, solvable: SolvableId) -> impl Display + '_ {
let s = self.pool.resolve_solvable(solvable);
format!("{}={}", self.pool.resolve_package_name(s.name), s.record)
}
fn display_name(&self, name: NameId) -> impl Display + '_ {
self.pool.resolve_package_name(name).clone()
}
fn display_version_set(&self, version_set: VersionSetId) -> impl Display + '_ {
format!("{}", self.pool.resolve_version_set(version_set))
}
fn display_string(&self, string_id: StringId) -> impl Display + '_ {
self.pool.resolve_string(string_id).to_owned()
}
fn version_set_name(&self, version_set: VersionSetId) -> NameId {
self.pool.resolve_version_set_package_name(version_set)
}
fn solvable_name(&self, solvable: SolvableId) -> NameId {
self.pool.resolve_solvable(solvable).name
}
fn version_sets_in_union(
&self,
version_set_union: VersionSetUnionId,
) -> impl Iterator<Item = VersionSetId> {
self.pool.resolve_version_set_union(version_set_union)
}
fn resolve_condition(&self, _condition: ConditionId) -> Condition {
unreachable!("no conditions in this reproducer")
}
}
impl DependencyProvider for ProvidesProvider {
async fn filter_candidates(
&self,
candidates: &[SolvableId],
version_set: VersionSetId,
inverse: bool,
) -> Vec<SolvableId> {
let range = self.pool.resolve_version_set(version_set);
candidates
.iter()
.copied()
.filter(|s| range.contains(&self.pool.resolve_solvable(*s).record) != inverse)
.collect()
}
async fn sort_candidates(&self, _solver: &SolverCache<Self>, solvables: &mut [SolvableId]) {
solvables.sort_by(|a, b| {
let a = self.pool.resolve_solvable(*a).record;
let b = self.pool.resolve_solvable(*b).record;
b.cmp(&a)
});
}
async fn get_candidates(&self, name: NameId) -> Option<Candidates> {
let providers = self.provides.get(&name)?;
Some(Candidates {
candidates: providers.clone(),
..Candidates::default()
})
}
async fn get_dependencies(&self, _solvable: SolvableId) -> Dependencies {
Dependencies::Known(KnownDependencies::default())
}
fn should_cancel_with_value(&self) -> Option<Box<dyn Any>> {
None
}
}
#[test]
fn virtual_capability_provided_by_multiple_package_names() {
let mut provider = ProvidesProvider::default();
// Two DIFFERENT package names both provide the capability "sysrel".
provider.add_provider("a", 1, "sysrel");
provider.add_provider("b", 1, "sysrel");
// Require the capability. Its version set's candidates are {a=1, b=1},
// which have different package names -> trips the debug_assert.
let requirement = provider.version_set("sysrel").into();
let problem = Problem::new().requirements(vec![requirement]);
let mut solver = Solver::new(provider);
let _ = solver.solve(problem);
}Observed: Real-world reproducer (resolvo-rpm)The same assert fires on real CentOS Stream metadata, because NotesFound while working on self-referential constrains (#230). It is independent of |
Version sets can represent virtual capabilities, so their candidates do not necessarily share one concrete package name. The optimization in #288 assumed they did. Debug builds trip the assertion, while release builds can put unrelated packages in the same at-most-one bucket and turn a valid solve into UNSAT.
This keeps the bulk registration path when all candidates have the same concrete name. Mixed-name candidate lists fall back to registering each candidate under its own name.
The regression test models a virtual capability provided by two packages and requires both concrete packages. It fails at the assertion in debug builds and returns UNSAT in release builds before the fix.
Testing
cargo fmt --checkcargo test --all-featurescargo test --release test_virtual_package_candidates_can_have_different_names